Feat/scaleset label change by yhaliaw · Pull Request #306 · canonical/github-runner-operators

yhaliaw · 2026-08-14T06:02:18Z

What this PR does

Add an ADR for this change. Recommend to read it before doing code review.
Do blue-green deployment style when making changes to the label of a scaleset.
There is backward compatibility, old naming style will be upgraded on next change to the scaleset

AI summary

Copilot kept crashing generating summary...

Why we need it

This closes the issue with changing labels for scaleset.

Checklist

`tox -e fmt` runs `ruff format` over `src`, which includes the Python snippet in the generated `garm_client_README.md`. The snippet had never been formatted, so `tox -e lint` (`ruff format --check`) failed on it regardless of what a change touched. This is generated output, so `scripts/generate_client.sh` will undo it on the next client regeneration — running `tox -e fmt` afterwards restores it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Labels are immutable once a GitHub scale set exists, so a label change in the garm-configurator relation data only logged a warning telling the operator to remove and re-add the relation. A label change is now applied by replacement. The live scaleset name carries a hash of its labels, so the name a spec should have is a pure function of the spec: every reconcile re-derives which live scaleset is current and which are replaced predecessors, with no charm-side state to persist, which is what makes the flow restart-safe and idempotent. The changeover is staged across reconciles and never blocks a hook: - the replacement is created first, so every label carried by both generations is served throughout; - the old scaleset is disabled only once the replacement is observed live, which closes its listener session and stops it receiving new jobs while runners already mid-job are left to finish; - it is deleted only after its runner count reaches zero. A scaleset draining past DRAIN_DEADLINE (7h, beyond GitHub's 6h job cap) holds a runner that is stuck rather than busy, so the delete is attempted regardless; GARM rejects the call while runners are genuinely active, so this cannot cut a job short but does stop a permanently faulted instance pinning a dead scaleset on GitHub. Progress is reported as a MaintenanceStatus naming the scaleset the operator configured and the phase it is in (creating replacement / draining N runners / awaiting deletion), returning to active once the replacement is serving and the old scaleset is gone. Also hardens the surrounding pass: a GarmApiError on one spec no longer aborts the others or the orphan sweep (it is re-raised after the pass, so the charm still reports the sync failed), a connection error still aborts immediately, and duplicate desired names are dropped rather than letting two specs retire each other's scaleset forever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The reconciler uses "spec" for a ScalesetSpec throughout, so "the desired specs" read as charm-side desired state rather than the GARM extra_specs the sentence is about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Pull request overview

This PR updates the garm charm’s scaleset reconciliation logic to support GitHub scaleset label changes via a blue/green-style replacement flow (create replacement, hand over, drain, delete), and documents the design decision via a new ADR.

Changes:

Implement label-hash-based “generation” naming for scalesets and a multi-reconcile replacement/drain/delete workflow in the scaleset reconciler.
Surface in-progress replacement state to Juju unit status and update integration/unit tests to assert the new behavior.
Add documentation (ADR + changelog) describing the rationale, operational behavior, and constraints.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
docs/changelog.md	Adds a user-facing changelog entry describing automatic scaleset label-change handling.
docs/adr/003_scaleset_label_change_by_replacement.md	New ADR documenting the replacement-based approach and its operational constraints.
charms/tests/integration/test_garm.py	Updates integration assertions/helpers to handle label-hashed live scaleset names.
charms/garm/tests/unit/test_scaleset_reconciler.py	Adds extensive unit coverage for replacement/drain semantics and edge cases.
charms/garm/tests/unit/test_garm_api.py	Adds unit tests for the new instances-listing client call used to gate deletion.
charms/garm/tests/unit/test_charm.py	Adds unit coverage for reporting replacement progress in Juju status.
charms/garm/src/scaleset_reconciler.py	Core implementation: generation naming, family resolution, replacement/drain orchestration, and defensive deletion gating.
charms/garm/src/garm_client_README.md	Updates the generated client README snippet (currently contains a garbled line in the snippet).
charms/garm/src/garm_api.py	Adds `list_scaleset_instances` wrapper around the Instances API for runner counting.
charms/garm/src/charm.py	Uses reconciler progress to emit maintenance status while replacements are in flight.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

florentianayuwono · 2026-08-17T06:48:19Z

+    digest = hashlib.sha256(logical_name.encode("utf-8")).hexdigest()[:LABEL_HASH_LENGTH]
+    return f"{logical_name[: limit - LABEL_HASH_LENGTH - 1]}-{digest}"


i understand the algo, just a question: if the goal of hashing is to avoid collision when the truncated name share the same prefix, then why not just return the hash itself for the entirety of the limit length

There's a name length limit i believe on GARM/GitHub side

florentianayuwono · 2026-08-17T07:11:55Z

+    if observed_name == logical_name:
+        return True


what about for the scalesets created before label-hashed naming that carries the unsuffixed name n also <= limit length? it will fall into this and be marked as true (shld it be false bc it's not generated?)

There is a code path for the unsuffixed legacy names.

florentianayuwono · 2026-08-17T08:10:22Z

+            The specs with duplicate names removed, first occurrence winning. Two
+            specs sharing a name would each own the other's live scaleset and retire
+            it on every reconcile, so they would replace each other forever.


forever as in forever? is there a way to just have one spec per a unique name?

I think this should be caused by multiple deployments using the same scaleset name. I do not think it can be avoided via charm code. In deployments, we should not be doing that.

yanksyoon

LGTM! Thanks @yhaliaw

yanksyoon · 2026-08-17T08:20:20Z

+        its deletion, which is a distinct thing to be waiting on.
+    """
+    if not progress.handed_over:
+        return "creating replacement"


Would it help separate the phases to be a constant or an enum?

Changed to constants.

yanksyoon · 2026-08-17T08:25:30Z

+                    self._reconcile_one(
+                        spec, providers, observed, templates, families.get(spec.name, [])
+                    )


Just out of curiosity, how long is this operation expected to take? I'm asking because if it's a long running operation, if there would be a way to do fire & forget?

yanksyoon · 2026-08-17T08:26:27Z

-            create_params = self._to_create_params(spec)
+            create_params = self._to_create_params(spec, active_name)
        except Exception as exc:
            logger.warning("Skipping scaleset %s: spec validation failed: %s", spec.name, exc)


Should this be an error log? Warning may go unnoticed it seems

Changed to error level.

yanksyoon · 2026-08-17T08:29:00Z

Just OOC, i'm guessing we won't test blue/green deployment since its a huge costly test?

It would be a long test. Maybe in the release gate?

cbartz

Read the ADR first as suggested — the design is sound and the naming/family logic is well covered by the unit tests. Two things I'd want resolved before this lands: a wire-format issue that I believe stops the changeover from ever completing, and one claim in the ADR that the cited GARM source doesn't actually establish. Details inline.

One non-blocking aside that didn't fit on a diff line — _needs_update compares observed.min_idle_runners != spec.min_idle_runners, and MinIdleRunners is omitempty in GARM (params/params.go:637), so a scaleset configured with min_idle_runners: 0 reads back as None and None != 0 makes the check true on every pass — a no-op update call each reconcile, forever. Pre-existing, not from this PR, and the same root cause as the first inline comment. Fine to split out.

This review was generated with AI assistance.

cbartz · 2026-08-17T08:34:54Z

+                    ScalesetProgress(spec.name, name, active_name, 0, handed_over=False)
+                )
+                continue
+            if old.enabled is not False:


if old.enabled is not False: self._retire(old)

I don't think this ever becomes False in production, which would mean the predecessor is never deleted.

GARM tags the field omitempty on the model this endpoint returns (params/params.go:642, swagger:model ScaleSet):

Enabled bool `json:"enabled,omitempty"`

There's no custom MarshalJSON, so a disabled scaleset comes back with no enabled key at all, and the generated client models it as Optional[StrictBool] = None (src/garm_client/models/scale_set.py:39).

So on the reconcile after cutover, old.enabled is None, which is not False → we re-enter _retire and continue, never reaching the runner-count check or _delete_drained. The scaleset stays disabled forever, the unit sits in MaintenanceStatus reporting "awaiting deletion", and the dead scale set stays registered on GitHub. DRAIN_DEADLINE can't rescue it either — that code is downstream of the same branch.

Inverting the test to the truthy form handles both shapes:

if old.enabled: self._retire(old)

replacement_live above is fine as-is — Go only omits the false case, so enabled is True still holds for an enabled scaleset.

Worth a quick confirmation against a live GARM, but the struct tag looks conclusive.

This review was generated with AI assistance.

This one should be fixed as well.

cbartz · 2026-08-17T08:34:54Z

+_NEW_NAME = target_scaleset_name("my-scaleset", _LABELS_NEW)
+
+
+def _generation(labels, **overrides):


This is why the suite doesn't catch the enabled issue I flagged in scaleset_reconciler.py.

_existing_scaleset sets enabled=True and every drain/delete test overrides it to enabled=False (e.g. test_draining_scaleset_is_not_deleted_while_runners_remain). Because of the omitempty behaviour on GARM's ScaleSet model, that's a state the real API can't return, so the whole retirement half of the suite is green against a shape that doesn't occur.

Could the fixture model the wire instead — drop the key when disabled rather than setting False? That would make these tests exercise the real path, and it'd guard the other omitempty fields on the same model from the same class of bug.

This review was generated with AI assistance.

cbartz · 2026-08-17T08:34:54Z

+3. While the predecessor reports runners, report progress and take no action.
+4. When it reports no runners, delete it and its runner template.
+
+Disabling closes the predecessor's listener session, ending job assignment to it.


Disabling closes the predecessor's listener session, ending job assignment to it.

The first half checks out, and more strongly than the ADR claims — listener.Stop() calls scaleSetClient.DeleteMessageSession(...) (workers/scaleset/scaleset_listener.go:123), so the session is deleted GitHub-side, not just dropped locally. handleScaleSetEvent stops it on !Enabled (scaleset.go:607) and keepListenerAlive won't restart it while disabled (sessionLoopMayRun, plus the two re-checks in the restart path). No argument there.

The second half — "ending job assignment to it" — is the part I can't find support for. The scale set still exists on GitHub after the disable, with the same labels in the same runner group, and assignment happens GitHub-side at queue time. Deleting the session stops GARM receiving; whether it stops GitHub assigning is undocumented GitHub behaviour that GARM's source can't tell us.

If GitHub does still assign, the failure mode is concrete: a job routed to the retiring scale set after the disable is never delivered, because GARM won't reopen the session to resume from last_message_id. It hangs until the scale set is deleted — up to DRAIN_DEADLINE by design. With both generations carrying overlapping labels for the whole drain, that wouldn't be a rare edge case.

This is load-bearing for the "no queue gap" argument, and it's the only one of the four listed behaviours without a code citation. Could we get either a reference or an observed run showing that jobs queued during a drain land on the replacement?

This review was generated with AI assistance.

This should be fixed now.

cbartz · 2026-08-17T08:34:54Z

+            return 0
+        try:
+            return len(self._client.list_scaleset_instances(scaleset.id))
+        except GarmApiError as exc:


reconcile's docstring says a connection error isn't contained ("GARM is down, so retrying every remaining spec would only stall the hook"), and reconcile re-raises it before the GarmApiError handler. But GarmConnectionError subclasses GarmApiError (garm_api.py:58), so the new drain helpers swallow it:

_remaining_runners (here) returns 1 and reports "still draining"

_retire logs and moves on

_delete_drained returns False

With GARM down mid-pass, the charm reports maintenance progress rather than "GARM sync failed". It self-corrects on retry, so this is non-blocking, but it masks the outage and diverges from the documented behaviour. An explicit except GarmConnectionError: raise ahead of each of these three would line them up with reconcile.

This review was generated with AI assistance.

The functions now raises the connection errors.

cbartz · 2026-08-17T08:34:54Z

+            # outlives this hook: report progress and let update-status converge it.
+            if replacing:
+                self.update_app_and_unit_status(
+                    ops.MaintenanceStatus(_scaleset_replacement_status(replacing))


Is maintenance the right status for a drain that can run for hours?

This puts app and unit into MaintenanceStatus for the whole drain, which the ADR notes can reach DRAIN_DEADLINE. The service is fully functional throughout — the replacement is serving, nothing is degraded — so anything that waits for active (integration tests, juju wait-for, upgrade orchestration) blocks on an unrelated background convergence.

Would ActiveStatus("Replacing scaleset …") communicate the same thing without that side effect? The ADR lists the consequence but doesn't argue the choice, so I may be missing a reason.

This review was generated with AI assistance.

Changed to active status.

cbartz · 2026-08-17T08:34:55Z


    scaleset = _wait_for_scaleset(base_url, token, _SCALESET_TEST_NAME)
-    assert scaleset["name"] == _SCALESET_TEST_NAME
+    assert re.fullmatch(


This adapts the existing assertion to hashed names, but nothing exercises the changeover itself. Given the design leans on GARM/GitHub behaviours that unit fixtures can't reproduce — the enabled comment on scaleset_reconciler.py being the case in point — I think one end-to-end pass is worth it.

It should be cheap here: the suite already runs GARM against the mock GitHub API (_point_github_endpoint_at_mock), and no VMs are involved — the provider binary is only checked for presence, so scalesets have zero instances and the drain completes immediately. Change the label in the configurator relation data, let update-status run twice, assert one enabled generation remains and the old one is gone. Roughly two extra hook cycles on an already-deployed model.

This review was generated with AI assistance.

I think we had a discussion on this. The team looking into the (ARC)[https://github.com/actions/actions-runner-controller] and GARM source code and it seems that disabling the scaleset was enough.

cbartz · 2026-08-17T08:34:55Z

+
+## Consequences
+
+Live names carry a hash suffix, so `garm-cli scaleset list` reports `my-scaleset-1a2b3c4d`.


One consequence worth adding: both generations carry the full min_idle_runners for the duration, so the idle runner count doubles during the drain. Against a fixed OpenStack quota the replacement may be unable to spawn runners at all — which would then also stall the changeover it's supposed to complete. Worth a line even if the answer is "operators should size for it".

This review was generated with AI assistance.

Added to ADR.

GARM tags ScaleSet.Enabled `omitempty` with no custom marshaller, so a disabled scaleset comes back with no `enabled` key and the generated client reads it as None. `old.enabled is not False` was therefore never false in production: the drain re-entered _retire on every pass and never reached the runner-count check or the delete, leaving the predecessor disabled forever and registered on GitHub. Compare truthily instead. The unit fixture set `enabled=False`, a shape the API cannot return, which is why the suite stayed green against it; it now drops the key like the wire does. The same comparison in the integration helper _find_scaleset counted a draining predecessor as still serving, and is fixed alongside. Also from review: - Re-raise GarmConnectionError from the three drain helpers. It subclasses GarmApiError, so the containing handlers reported drain progress while GARM was down, diverging from reconcile()'s documented behaviour. - Report the drain as active rather than maintenance. Every label is served throughout, so blocking `juju wait-for` and integration tests on hours of healthy background convergence is the wrong trade. - Cover the changeover end to end in the integration suite: no VMs are involved, so the drain completes as fast as the charm reconciles. - Log a malformed spec at error level; unlike the provider/entity gates it will not resolve on its own. - Name the replacement phases; apply the review suggestion for `claimed`. - ADR: claim only what the GARM source establishes about the listener session, and record the GitHub-side job-assignment question as open. Note that both generations carry min_idle_runners for the whole drain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

yhaliaw and others added 6 commits August 14, 2026 13:47

Merge branch 'main' into feat/scaleset-label-change

8f4149b

Add ADR

f3650b2

Update the comments

3741cbf

style(garm): disambiguate "specs" in the extra_specs comment

b834c4f

The reconciler uses "spec" for a ScalesetSpec throughout, so "the desired specs" read as charm-side desired state rather than the GARM extra_specs the sentence is about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

yhaliaw marked this pull request as ready for review August 14, 2026 08:04

yhaliaw requested review from cbartz, florentianayuwono, javierdelapuente, weiiwang01 and yanksyoon as code owners August 14, 2026 08:04

Merge branch 'main' into feat/scaleset-label-change

7ff0d89

florentianayuwono requested a lite review from Copilot August 17, 2026 02:58

Copilot started reviewing on behalf of florentianayuwono August 17, 2026 02:58 View session

Copilot AI reviewed Aug 17, 2026

View reviewed changes

Comment thread charms/garm/tests/unit/test_scaleset_reconciler.py Outdated

Comment thread charms/garm/src/garm_client_README.md

Potential fix for pull request finding

103499f

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

florentianayuwono reviewed Aug 17, 2026

View reviewed changes

yanksyoon approved these changes Aug 17, 2026

View reviewed changes

cbartz reviewed Aug 17, 2026

View reviewed changes

yhaliaw and others added 2 commits August 18, 2026 10:27

Merge branch 'main' into feat/scaleset-label-change

c1460bf

		digest = hashlib.sha256(logical_name.encode("utf-8")).hexdigest()[:LABEL_HASH_LENGTH]
		return f"{logical_name[: limit - LABEL_HASH_LENGTH - 1]}-{digest}"

		_NEW_NAME = target_scaleset_name("my-scaleset", _LABELS_NEW)


		def _generation(labels, **overrides):


		## Consequences

		Live names carry a hash suffix, so `garm-cli scaleset list` reports `my-scaleset-1a2b3c4d`.

Uh oh!

Conversation

yhaliaw commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

AI summary

Why we need it

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanksyoon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbartz left a comment

Choose a reason for hiding this comment

Uh oh!

cbartz Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbartz Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbartz Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbartz Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbartz Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbartz Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

yhaliaw commented Aug 14, 2026 •

edited

Loading

cbartz Aug 17, 2026 •

edited

Loading

cbartz Aug 17, 2026 •

edited

Loading

cbartz Aug 17, 2026 •

edited

Loading

cbartz Aug 17, 2026 •

edited

Loading

cbartz Aug 17, 2026 •

edited

Loading

cbartz Aug 17, 2026 •

edited

Loading

cbartz Aug 17, 2026 •

edited

Loading